PKCE configuration - enabled by default #17507

rohan-naik07 · 2025-07-10T15:53:23Z

PKCE enabled by default for confidential as well as public clients.
Client Authentication method won't affect the PKCE customizer.
PKCE can be disabled using isRequireProofKey() client setting.

rohan-naik07 · 2025-07-17T15:55:52Z

Hi @jgrandja ,

Thank you for reviewing my pull request. I noticed that you marked it as "breaks passivity" and self-assigned the related issue.

I want to understand this better so I can improve future contributions. Could you please clarify what aspect of the PR breaks passivity, and how you envision the correct approach? I'm eager to align with the design principles of the project and contribute effectively.

Thanks again for your time and guidance!

Best regards,
Rohan

jgrandja · 2025-07-18T10:56:44Z

Hi @rohan-naik07. I haven't had time to review your PR but I plan on it next week.

Our process is to self-assign PR's when we review it and the user who submitted the PR is assigned the original issue.

The reason I marked this "breaks-passivity" is because it is a breaking change that needs to be applied. For example, since the required change is for the OAuth2 Client to send PKCE parameters by default, it's possible that the authorization server does not support PKCE and therefore the flow will fail. So what was working previously might stop working with the upgrade to 7.0 because of this change. Does that make sense?

FYI, the reason for this change is OAuth 2.1 recommends/requires PKCE.

rohan-naik07 · 2025-07-19T08:01:25Z

Ok, that makes sense. Thanks for clarifying my doubts.

jgrandja

Thanks for the PR @rohan-naik07. Please see review comments.

Also, please rebase off of main and ensure there is only 1 commit with the changes. Thanks.

jgrandja · 2025-08-13T09:04:33Z

...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java

@@ -655,6 +655,10 @@ private ClientRegistration create() {
 			clientRegistration.clientName = StringUtils.hasText(this.clientName) ? this.clientName
 					: this.registrationId;
 			clientRegistration.clientSettings = this.clientSettings;
+			if (this.clientSettings.requireProofKey) {


Please remove this since clientRegistration.clientSettings is set in previous line with this.clientSettings and only the clientSettings should be used.

jgrandja · 2025-08-13T09:06:52Z

...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java

-		private ClientSettings() {
-
-		}
+		private ClientSettings() {}


Please revert this formatting change as only required changes should be applied

jgrandja · 2025-08-13T09:09:48Z

...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java

@@ -794,6 +790,7 @@ public static final class Builder {
 			private boolean requireProofKey;

 			private Builder() {
+				this.requireProofKey = true;


Please move this to variable initialization private boolean requireProofKey = true;

jgrandja · 2025-08-13T09:14:02Z

...rg/springframework/security/oauth2/client/web/DefaultOAuth2AuthorizationRequestResolver.java

@@ -184,8 +183,7 @@ private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientR
 				// value.
 				applyNonce(builder);
 			}
-			if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())
-					|| clientRegistration.getClientSettings().isRequireProofKey()) {
+			if (clientRegistration.getClientSettings().isRequireProofKey()) {


Public Clients (indicated with ClientAuthenticationMethod.NONE) always require PKCE but with this change it's possible to disable PKCE for Public Clients. Please revert this as the original logic is still correct.

jgrandja · 2025-08-13T09:20:07Z

...ework/security/oauth2/client/web/server/DefaultServerOAuth2AuthorizationRequestResolver.java

@@ -196,8 +195,7 @@ private OAuth2AuthorizationRequest.Builder getBuilder(ClientRegistration clientR
 				// value.
 				applyNonce(builder);
 			}
-			if (ClientAuthenticationMethod.NONE.equals(clientRegistration.getClientAuthenticationMethod())
-					|| clientRegistration.getClientSettings().isRequireProofKey()) {
+			if (clientRegistration.getClientSettings().isRequireProofKey()) {


Public Clients (indicated with ClientAuthenticationMethod.NONE) always require PKCE but with this change it's possible to disable PKCE for Public Clients. Please revert this as the original logic is still correct.

jgrandja · 2025-08-13T09:26:11Z

docs/modules/ROOT/pages/servlet/oauth2/client/client-authentication.adoc

@@ -293,6 +293,6 @@ spring:

 [NOTE]
 ====
-Public Clients are supported using https://tools.ietf.org/html/rfc7636[Proof Key for Code Exchange] (PKCE).
-PKCE will automatically be used when `client-authentication-method` is set to "none" (`ClientAuthenticationMethod.NONE`).
+https://tools.ietf.org/html/rfc7636[Proof Key for Code Exchange] (PKCE) will be enabled by default for public as well as confidential clients. Refer https://docs.spring.io/spring-security/reference/servlet/oauth2/client/client-authentication.html#oauth2-client-authentication-public[this


Please revert this as the original content is still valid

jgrandja · 2025-08-13T09:31:41Z

docs/modules/ROOT/pages/servlet/oauth2/client/core.adoc

@@ -69,7 +69,8 @@ This information is available only if the Spring Boot property `spring.security.
 <15> `(userInfoEndpoint)authenticationMethod`: The authentication method used when sending the access token to the UserInfo Endpoint.
 The supported values are *header*, *form*, and *query*.
 <16> `userNameAttributeName`: The name of the attribute returned in the UserInfo Response that references the Name or Identifier of the end-user.
-<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: If `true` or if `authorizationGrantType` is `none`, then PKCE will be enabled by default.
+<17> [[oauth2Client-client-registration-requireProofKey]]`requireProofKey`: This will by default be `true`, as PKCE will be enabled by default.


Please update the text to:

If true or if clientAuthenticationMethod is none, then PKCE will be enabled.

Aren't we stressing enough on the point that PKCE will be also enabled for confidential clients?... Or we don't want to for now

jgrandja · 2025-08-13T09:36:11Z

...st/java/org/springframework/security/oauth2/client/registration/TestClientRegistrations.java

@@ -64,6 +64,22 @@ public static ClientRegistration.Builder clientRegistration2() {
 		// @formatter:on
 	}

+	public static ClientRegistration.Builder publicClientRegistrationWithNoPkce() {


Please remove this as TestClientRegistrations.clientRegistration() can be used and the caller can simply change builder.clientSettings(updatedClientSettings)

Signed-off-by: Rohan Naik <[email protected]>

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 10, 2025

rohan-naik07 changed the base branch from main to 6.5.x July 10, 2025 16:55

rohan-naik07 marked this pull request as ready for review July 14, 2025 14:29

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: breaks-passivity A change that breaks passivity with the previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 15, 2025

jgrandja self-assigned this Jul 15, 2025

jgrandja requested changes Aug 13, 2025

View reviewed changes

rohan-naik07 force-pushed the pkce-default-config-gh-16391 branch from 4d50c65 to 119e5df Compare August 16, 2025 10:21

PKCE configuration - enabled by default

abe7742

Signed-off-by: Rohan Naik <[email protected]>

rohan-naik07 force-pushed the pkce-default-config-gh-16391 branch from 119e5df to abe7742 Compare August 18, 2025 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

PKCE configuration - enabled by default #17507

PKCE configuration - enabled by default #17507

rohan-naik07 commented Jul 10, 2025 •

edited

Loading

Uh oh!

rohan-naik07 commented Jul 17, 2025

Uh oh!

jgrandja commented Jul 18, 2025

Uh oh!

rohan-naik07 commented Jul 19, 2025

Uh oh!

jgrandja left a comment

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

rohan-naik07 Aug 15, 2025

Uh oh!

jgrandja Aug 13, 2025

Uh oh!

Uh oh!

PKCE configuration - enabled by default #17507

Are you sure you want to change the base?

PKCE configuration - enabled by default #17507

Conversation

rohan-naik07 commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohan-naik07 commented Jul 17, 2025

Uh oh!

jgrandja commented Jul 18, 2025

Uh oh!

rohan-naik07 commented Jul 19, 2025

Uh oh!

jgrandja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rohan-naik07 commented Jul 10, 2025 •

edited

Loading